feat(vnext): coordinate catalog epochs#193
Merged
Merged
Conversation
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
Introduces a vnext “catalog epoch coordinator” that serializes and arbitrates catalog invalidations and response epochs per scope, preventing mixed catalog state and providing bounded, fail-closed behavior under hostile/reentrant providers.
Changes:
- Adds
createSqlCatalogEpochCoordinatorand related types/constants to coordinate scope memberships, epoch captures, invalidations, and response submissions with bounded FIFO draining. - Refines
SqlDisposable.disposeto be athis-free callback ((this: void) => void) and updates type-level tests to enforce it. - Adds extensive vitest coverage plus a vitest benchmark and a new
bench:catalog-coordinatornpm script; updates ADR 0005 with lifecycle/ordering/resource-contract details.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/vnext-types/marimo-relation-completion.test-d.ts | Adds type-level assertion that SqlDisposable.dispose cannot be receiver-dependent. |
| src/vnext/relation-completion-types.ts | Updates SqlDisposable.dispose signature to (this: void) => void. |
| src/vnext/relation-catalog-epoch-coordinator.ts | New epoch coordinator implementation with bounded command draining, scope subscriptions, capture authentication, and fail-closed cleanup/quarantine behavior. |
| src/vnext/tests/relation-catalog-epoch-coordinator.test.ts | Comprehensive test suite covering ordering, hostile behaviors, bounds, and disposal semantics. |
| src/vnext/tests/relation-catalog-epoch-coordinator.bench.ts | Adds benchmarks and preflight assertions for coordinator fan-out, churn, and callback storm behavior. |
| package.json | Adds bench:catalog-coordinator script entry. |
| docs/adr/0005-parser-independent-relation-completion.md | Documents the coordinator’s lifecycle, ordering, and resource limits in ADR 0005. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/vnext/relation-catalog-epoch-coordinator.ts">
<violation number="1" location="src/vnext/relation-catalog-epoch-coordinator.ts:381">
P2: A hostile rejected native cleanup Promise can still escape as an unhandled rejection when it shadows `then`, because this assimilation path does not attach a handler to the original Promise. The cleanup isolation should use an intrinsic native-Promise reaction for native Promises (including poisoned own `then` properties) before falling back to thenable assimilation.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| try { | ||
| const result = Reflect.apply(cleanup, undefined, []); | ||
| const settlement = new Promise<unknown>((resolve) => { | ||
| resolve(result); |
There was a problem hiding this comment.
P2: A hostile rejected native cleanup Promise can still escape as an unhandled rejection when it shadows then, because this assimilation path does not attach a handler to the original Promise. The cleanup isolation should use an intrinsic native-Promise reaction for native Promises (including poisoned own then properties) before falling back to thenable assimilation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/vnext/relation-catalog-epoch-coordinator.ts, line 381:
<comment>A hostile rejected native cleanup Promise can still escape as an unhandled rejection when it shadows `then`, because this assimilation path does not attach a handler to the original Promise. The cleanup isolation should use an intrinsic native-Promise reaction for native Promises (including poisoned own `then` properties) before falling back to thenable assimilation.</comment>
<file context>
@@ -375,7 +376,11 @@ function cleanupSubscription(subscription: SubscriptionState): void {
- Reflect.apply(cleanup, undefined, []);
+ const result = Reflect.apply(cleanup, undefined, []);
+ const settlement = new Promise<unknown>((resolve) => {
+ resolve(result);
+ });
+ void settlement.then(undefined, IGNORE_CLEANUP_REJECTION);
</file context>
This was referenced Jul 25, 2026
Light2Dark
added a commit
that referenced
this pull request
Jul 25, 2026
Part of #169. Follows #193. - Add a package-private synchronous epoch-transition hook so shared catalog work can retire before session revision listeners observe an accepted epoch. - Preserve deterministic prepare/commit/dispatch ordering for provider invalidations and higher response epochs. - Tighten provider cleanup and transition dispatch to an exact `undefined` contract, with fail-closed quarantine for invalid returns. - Dispose before inspecting invalid thenables, closing the hostile getter reentrancy window identified after #193 merged. - Keep the no-hook and null-transition paths allocation-free and add a configured-hook storm benchmark. - Extend ADR 0005, type fixtures, lifecycle tests, and adversarial Promise/thenable coverage. <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Adds a package-private, synchronous epoch-transition hook that runs after epoch install to retire catalog work before session listeners. Enforces exact-return contracts and fail-closed handling to harden reentrancy, cleanup, and poisoned-thenable edge cases; supports #169. - **New Features** - Added `SqlCatalogEpochTransitionTarget` that may return a dispatch closure or `null`; dispatch must return exactly `undefined` (sync only). - Preserved strict ordering: transition-prepare → revision-prepare → producer settle → transition-dispatch → revision-dispatch; reentrant dispatch queues behind current dispatch. - Snapshots the revision audience before transition; producers/aborts see prepared revisions; listeners see already-retired work. - Rejects non-function transition targets during coordinator creation (`reason: "invalid-transition-target"`); any thrown preparation, thrown dispatch, or non-`undefined` return disposes fail-closed and discards response decisions. - Drains detached Promise/thenable results for cleanup and transition returns after disposal, avoiding hostile getter reentrancy; no-hook and `null`-dispatch paths allocate nothing. Updated ADR 0005, added a configured-hook storm benchmark, and expanded adversarial tests. - **Migration** - Update `SqlCatalogSubscriptionCleanup` to be synchronous and return `undefined` only (no Promises or other return values). <sup>Written for commit 9d7c9c3. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/marimo-team/codemirror-sql/pull/194?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img](https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a){kind=link}
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #169.
Summary by cubic
Introduces a catalog epoch coordinator that gates responses and invalidations per scope to prevent mixed catalog state and ensure deterministic revision fan‑out, and isolates hostile cleanup promises. Part of #169.
New Features
bench:catalog-coordinator.Migration
SqlRelationCatalogProvider.subscribenow returns a this‑free cleanup functionSqlCatalogSubscriptionCleanup(may be async) instead of aSqlDisposable; update providers to return() => void | PromiseLike<void>.SqlDisposable.disposeis now this‑free: change todispose(this: void): voidand avoid usingthisinside dispose.Written for commit 3d66bd4. Summary will update on new commits.